Restructure parse_dandi_url() return type #605
Conversation
Codecov Report
@@ Coverage Diff @@
## master #605 +/- ##
==========================================
+ Coverage 83.08% 84.16% +1.08%
==========================================
Files 59 59
Lines 5681 5778 +97
==========================================
+ Hits 4720 4863 +143
+ Misses 961 915 -46
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
| @@ -110,11 +110,11 @@ def ls(paths, schema, metadata, fields=None, format="auto", recursive=False, job | |||
| def assets_gen(): | |||
| for path in paths: | |||
| if is_url(path): | |||
| from ..dandiarchive import navigate_url | |||
There was a problem hiding this comment.
could you please add a test which would cover this code path? I have added raising an exception here and none of the tests flipped anyhow (directly via python or may be through external invocation of dandi ls)
There was a problem hiding this comment.
Should the test just be a smoke test that calls ls with a URL (pointing to dandiarchive.org or the Docker instance?), or should it also expect something from the output?
There was a problem hiding this comment.
I guess with -f json_pp you could expect nicely formatted record you could loads and if ran on a dandiset with assets -- could check that there is at least one asset (without getting into details)
There was a problem hiding this comment.
by -f json_pp I didn't mean "run in command line"... format="json_pp" ;)
| from urllib.parse import unquote as urlunquote | ||
|
|
||
| from pydantic import AnyHttpUrl, BaseModel, parse_obj_as, validator |
There was a problem hiding this comment.
just a note: likely because of this module RF and extra imports (but didn't investigate), code of this PR adds ~40 msec to dandi --help (to the total of 800ms, so about 5%)... at some point we might review import paths etc if we keep growing - since waiting seconds for --help might be a bit too much, and would be impediment if we add some shell completions support (IIRC I did not find any ready to use for click such as argcomplete which was odd)
There was a problem hiding this comment.
FYI, click has command completion built-in: https://click.palletsprojects.com/en/7.x/bashcomplete/ (though I think some details may be changing in Click 8).
There was a problem hiding this comment.
cool, filed #608 . Feel welcome to self-assign but it is of no urgency so I didn't
|
Thank you @jwodder -- let's proceed |
Closes #554.
Closes #598.